opt: add support for numeric references #28159
Conversation
|
This change is |
madhavsuresh
force-pushed
the
numeric_references
branch
2 times, most recently
from
d78d2db to
ccf810b
Compare
madhavsuresh
changed the title
madhavsuresh
requested review from
andy-kimball,
rytaft,
RaduBerinde,
justinj and
a team
and removed request for
andy-kimball and
rytaft
madhavsuresh
force-pushed
the
numeric_references
branch
from
ccf810b to
2aca2b1
Compare
There was a problem hiding this comment.
Looks good overall!
Reviewable status:
complete! 0 of 0 LGTMs obtained
pkg/sql/opt_catalog.go, line 54 at r1 (raw file):
} func (oc *optCatalog) FindTableByTableRef(ctx context.Context, tableID int64) (opt.Table, error) {
[nit] add // FindTableByTableRef is part of the .. interface.
pkg/sql/opt_catalog.go, line 62 at r1 (raw file):
} dbDesc, err := sqlbase.GetDatabaseDescFromID(ctx, oc.resolver.Txn(), desc.ParentID)
err not checked
pkg/sql/opt_catalog.go, line 240 at r1 (raw file):
} // LookupColumnOrdinal exposes optTable.lookupColumnOrdinal.
Most of this comment belongs inside the function (the caller should not care about private internals). I'd just say // LookupColumnOrdinal is part of the opt.Table interface.
pkg/sql/opt_catalog.go, line 57 at r2 (raw file):
// TODO(madhavsuresh): this does not completely mirror planner.getTableDescByID. // It does not use the descriptorCache desc, err := sqlbase.GetTableDescFromID(ctx, oc.resolver.Txn(), sqlbase.ID(tableID))
It's probably better if this path avoids the cache, given that it's used for testing. @knz should confirm.
pkg/sql/opt/catalog.go, line 198 at r1 (raw file):
Column(i int) Column // LookupColumnOrdinal returns the ordinal of the column with the given ID. A
[nit] mention that this is the internal column ID and has no relation to ColumnIDs in the optimizer. I'd remove the comment about the cache.
pkg/sql/opt/catalog.go, line 226 at r2 (raw file):
// given table name. Returns an error if the table does not exist. FindTable(ctx context.Context, name *tree.TableName) (Table, error) FindTableByTableRef(ctx context.Context, tableID int64) (Table, error)
Blank line and add a comment. Consider renaming to FindTableByID
pkg/sql/opt/exec/execbuilder/testdata/select, line 175 at r2 (raw file):
query error pq: cannot use "\*" without a FROM clause SELECT * FROM [53() AS num_ref_alias]
Would SELECT 1 FROM [53() AS num_ref_alias] work?
pkg/sql/opt/testutils/testcat/test_catalog.go, line 48 at r1 (raw file):
) func (tc *Catalog) FindTableByTableRef(ctx context.Context, tableID int64) (opt.Table, error) {
[nit] add a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt_catalog.go, line 54 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] add
// FindTableByTableRef is part of the .. interface.
Done.
pkg/sql/opt_catalog.go, line 62 at r1 (raw file):
Previously, RaduBerinde wrote…
err not checked
Done.
pkg/sql/opt_catalog.go, line 240 at r1 (raw file):
Previously, RaduBerinde wrote…
Most of this comment belongs inside the function (the caller should not care about private internals). I'd just say
// LookupColumnOrdinal is part of the opt.Table interface.
Done.
pkg/sql/opt/catalog.go, line 198 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] mention that this is the internal column ID and has no relation to ColumnIDs in the optimizer. I'd remove the comment about the cache.
Done.
pkg/sql/opt/catalog.go, line 226 at r2 (raw file):
Previously, RaduBerinde wrote…
Blank line and add a comment. Consider renaming to
FindTableByID
Done. Renamed.
pkg/sql/opt/exec/execbuilder/testdata/select, line 175 at r2 (raw file):
Previously, RaduBerinde wrote…
Would
SELECT 1 FROM [53() AS num_ref_alias]work?
Yea it does. Does what you'd expect, returns 1 per row.
pkg/sql/opt/testutils/testcat/test_catalog.go, line 48 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] add a comment
Done.
madhavsuresh
force-pushed
the
numeric_references
branch
2 times, most recently
from
ab0bc27 to
861da08
Compare
There was a problem hiding this comment.
Great stuff!! Most of my comments are nits.
Reviewed 3 of 9 files at r1, 5 of 5 files at r3, 1 of 1 files at r4.
Reviewable status:
pkg/sql/opt_catalog.go, line 54 at r3 (raw file):
} // FindTableByID is part of the Catalog interface
[nit] period at end of function comment. Here and below.
pkg/sql/opt_catalog.go, line 73 at r3 (raw file):
wrapper, ok := oc.wrappers[desc] if !ok { // Is this a normalized table name?
Not sure I understand this comment - I think the comment should be:
// Make fully qualified table name.
pkg/sql/opt_catalog.go, line 88 at r3 (raw file):
} if err := oc.resolver.CheckPrivilege(ctx, desc, privilege.SELECT); err != nil {
Do you need to add a CheckPrivilege check in FindTableByID too?
pkg/sql/opt/catalog.go, line 200 at r4 (raw file):
// LookupColumnOrdinal returns the ordinal of the column with the given ID. // Note that this takes the internal column ID, and has no relation to // ColumnIDs in the optimizer
[nit] add period
pkg/sql/opt/catalog.go, line 228 at r4 (raw file):
FindTable(ctx context.Context, name *tree.TableName) (Table, error) // FindTableByID returns Table interface for the database table
[nit] Table -> a Table
pkg/sql/opt/exec/execbuilder/testdata/select, line 161 at r4 (raw file):
· spans ALL · · query error pq: descriptor not found
does this match the error message of the heuristic planner? seems like "error: relation [666] does not exist" would be more helpful than "descriptor not found". Then maybe you could use the same tests as the heuristic planner?
pkg/sql/opt/exec/execbuilder/testdata/select, line 184 at r4 (raw file):
query III SELECT * FROM [53(4,3,1) AS num_ref_alias]
Did you add these logic tests here because they don't work in the heuristic planner? If so, maybe add a comment to that effect.
pkg/sql/opt/exec/execbuilder/testdata/select, line 205 at r4 (raw file):
query III SELECT * FROM [53(1,3,4) AS num_ref_alias(col1,col2,col3)]
maybe add test directive "colnames" to check that the column names are correct
pkg/sql/opt/optbuilder/select.go, line 149 at r4 (raw file):
func (b *Builder) buildScanWithTableRef(tab opt.Table, tn *tree.TableName, inScope *scope, ref *tree.TableRef) (outScope *scope) {
[nit] usually we break long lines at the parentheses. e.g.:
func (b *Builder) buildScanWithTableRef(
...
) (outScope *scope) {
pkg/sql/opt/optbuilder/select.go, line 155 at r4 (raw file):
var colsToAdd []int // See tree.TableRef: "Note that a nil [Columns] array means 'unspecified' (all columns)."
[nit] 80 columns for comments
pkg/sql/opt/optbuilder/testdata/select, line 1123 at r4 (raw file):
# These columns are off by one from what # the logictests will run. This is due # to the implementation of the test_catalog
Is it easy to fix test_catalog to do the right thing? That seems better than having off-by-one tests.
pkg/sql/opt/testutils/testcat/test_catalog.go, line 48 at r4 (raw file):
) func (tc *Catalog) FindTableByID(ctx context.Context, tableID int64) (opt.Table, error) {
Still needs a comment
|
@BramGruneir could you have a look at this on behalf of the sql frontend team, I'm really swamped |
madhavsuresh
force-pushed
the
numeric_references
branch
3 times, most recently
from
46ce3d5 to
0b000bb
Compare
|
pkg/sql/opt/exec/execbuilder/testdata/select, line 175 at r2 (raw file): Previously, madhavsuresh wrote…
@knz the query I asked for was |
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/optbuilder/select.go, line 148 at r6 (raw file):
} func (b *Builder) buildScanWithTableRef(
[nit] would be good to have a comment with a sample SQL query, given that this syntax is fairly obscure
pkg/sql/opt/testutils/testcat/create_table.go, line 115 at r6 (raw file):
} // We need to keep track of the tableID from
[nit] reformat at 80 cols
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/drop_table, line 36 at r6 (raw file):
Previously, madhavsuresh wrote…
The table above was dropped, I wanted to test this code path: https://github.com/cockroachdb/cockroach/pull/28159/files#diff-009df3bea1535b5639b1af5a45ff44f9R72
thanks.
pkg/sql/opt/exec/execbuilder/testdata/select, line 175 at r2 (raw file):
Previously, RaduBerinde wrote…
@knz the query I asked for was
SELECT 1 FROM [53() AS num_ref_alias], where a 1 per row is correct.
oh thanks for clarifying.
pkg/sql/opt/exec/execbuilder/testdata/select, line 184 at r4 (raw file):
Previously, madhavsuresh wrote…
Thanks for the comments @knz! That sounds good to me as well. That's a very easy change to make within the existing planner.
Yes if you can make both code paths fail with the same error, go ahead.
madhavsuresh
force-pushed
the
numeric_references
branch
4 times, most recently
from
5ec7f8e to
66c7940
Compare
There was a problem hiding this comment.
I have kept the tests in opt/ as well as the table_ref_tests.go. Though, it seems that perhaps the opt/ tests are no longer necessary?
Reviewable status:
pkg/sql/opt_catalog.go, line 57 at r2 (raw file):
Previously, knz (kena) wrote…
Yes please remove the TODO, explain in the comment that this bypasses the cache, and call the cache bypassing out in the release note.
Done.
pkg/sql/opt_catalog.go, line 88 at r3 (raw file):
Previously, knz (kena) wrote…
Well to be fully precise the current way the
optcode tests privileges is preparing us for a world of hurt when this needs to be extended to support mutations and DDL.At the very least add a TODO here and above to mention this is not a satisfactory state of affairs. Also maybe file an issue (unless there is already one, @justinj?) to clean this up later and refer to that issue in the TODO.
Done. Added a TODO. Will follow up with @justinj on issues.
pkg/sql/opt_catalog.go, line 72 at r6 (raw file):
Previously, rytaft wrote…
Do we need to add this check to
FindTableas well?
I would think so. I can add that in a separate PR.
pkg/sql/opt_catalog.go, line 275 at r6 (raw file):
Previously, knz (kena) wrote…
please consider
pgerror.NewErrorfhere with a suitable error code.
Done.
pkg/sql/table_ref_test.go, line 39 at r6 (raw file):
Previously, knz (kena) wrote…
Two things:
do not believe that should be needed -- if the rest of your PR is correct, this test should pass with the optimizer code, and the errors should be the same
if you indeed need to disable the optimizer here, then you must also implement the corresponding set of unit tests for the code in your PR.
Given our discussion below, the opt code now passes the table_ref_test.
pkg/sql/opt/exec/execbuilder/testdata/select, line 184 at r4 (raw file):
Previously, knz (kena) wrote…
Yes if you can make both code paths fail with the same error, go ahead.
Done.
pkg/sql/opt/exec/execbuilder/testdata/select, line 186 at r4 (raw file):
Previously, knz (kena) wrote…
Oh and by the way an unrelated point: if a column is marked hidden in the table descriptor, then selecting that column using a numeric reference should also yield a hidden column in the logical plan. Can you add a test for that?
Thanks for catching this. Added tests for this.
pkg/sql/opt/optbuilder/select.go, line 148 at r6 (raw file):
Previously, RaduBerinde wrote…
[nit] would be good to have a comment with a sample SQL query, given that this syntax is fairly obscure
Done.
pkg/sql/opt/testutils/testcat/create_table.go, line 115 at r6 (raw file):
Previously, RaduBerinde wrote…
[nit] reformat at 80 cols
Done.
pkg/sql/opt/testutils/testcat/test_catalog.go, line 55 at r6 (raw file):
Previously, knz (kena) wrote…
- use
pgerror.NewErrorf- make the error the same as for regular names,
relation [%d] not found.
Done.
pkg/sql/opt/testutils/testcat/test_catalog.go, line 222 at r6 (raw file):
Previously, knz (kena) wrote…
ditto error code
Done.
madhavsuresh
force-pushed
the
numeric_references
branch
from
66c7940 to
05a02ed
Compare
There was a problem hiding this comment.
Reviewed 9 of 9 files at r7.
Reviewable status:
pkg/sql/data_source.go, line 227 at r7 (raw file):
if tref.Columns != nil && len(tref.Columns) == 0 { return planDataSource{}, pgerror.NewErrorf(pgerror.CodeSyntaxError, "An explicit list of column IDs must include at least one column")
[nit] I think error messages are supposed to start with a lower-case letter
pkg/sql/opt_catalog.go, line 88 at r3 (raw file):
Previously, madhavsuresh wrote…
Done. Added a TODO. Will follow up with @justinj on issues.
I'd add a TODO on both calls to CheckPrivileges, just to make sure we don't miss it.
pkg/sql/opt_catalog.go, line 72 at r6 (raw file):
Previously, madhavsuresh wrote…
I would think so. I can add that in a separate PR.
👍
pkg/sql/table_ref_test.go, line 117 at r7 (raw file):
{fmt.Sprintf("[%d(%d) as t]@[%d]", tID, bID, secID), `(d)`, ``}, {fmt.Sprintf("[%d(%d) as t]@[%d]", tID, cID, secID), `(c)`, ``}, {fmt.Sprintf("[%d(%d) as t]@[%d]", tID, cID, secID), `(c)`, ``},
duplicate test
pkg/sql/opt/exec/execbuilder/testdata/select, line 184 at r4 (raw file):
Previously, madhavsuresh wrote…
Done.
Looks good - now you can remove these tests (or move them to logictests if they are not there already)
pkg/sql/opt/optbuilder/select.go, line 92 at r7 (raw file):
case *tree.TableRef: if source.Columns != nil && len(source.Columns) == 0 {
[nit] I would move this check inside buildScanWithTableRef so the code is all contained there. Putting it here does save a call to resolveTableRef, but there's not much benefit to optimizing for the error case.
pkg/sql/opt/optbuilder/testdata/select, line 1209 at r7 (raw file):
SELECT * FROM [54(3) AS t] ---- panic: presentation property doesn't apply to non-relational operators
Why is this causing a panic? I think this case should act just like SELECT * FROM no_cols_table
It would be good to also add one test case like this:
SELECT rowid FROM [54(3) AS t]
madhavsuresh
force-pushed
the
numeric_references
branch
from
05a02ed to
10af75e
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/data_source.go, line 227 at r7 (raw file):
Previously, rytaft wrote…
[nit] I think error messages are supposed to start with a lower-case letter
Done.
pkg/sql/opt_catalog.go, line 88 at r3 (raw file):
Previously, rytaft wrote…
I'd add a TODO on both calls to CheckPrivileges, just to make sure we don't miss it.
Done.
pkg/sql/table_ref_test.go, line 117 at r7 (raw file):
Previously, rytaft wrote…
duplicate test
Done.
pkg/sql/opt/exec/execbuilder/testdata/select, line 184 at r4 (raw file):
Previously, rytaft wrote…
Looks good - now you can remove these tests (or move them to logictests if they are not there already)
Done.
pkg/sql/opt/optbuilder/select.go, line 92 at r7 (raw file):
Previously, rytaft wrote…
[nit] I would move this check inside
buildScanWithTableRefso the code is all contained there. Putting it here does save a call toresolveTableRef, but there's not much benefit to optimizing for the error case.
Done.
pkg/sql/opt/optbuilder/testdata/select, line 1209 at r7 (raw file):
Previously, rytaft wrote…
Why is this causing a panic? I think this case should act just like
SELECT * FROM no_cols_tableIt would be good to also add one test case like this:
SELECT rowid FROM [54(3) AS t]
There's a panic as well with a table with no columns in optbuilder:
exec-ddl
CREATE TABLE no_cols_table ()
----
build
SELECT * FROM no_cols_table
----
The panic is here:
For now I've removed this test from optbuilder, since it causes the tests to panic out.
Thanks for that suggestion - added the test.
There was a problem hiding this comment.
Reviewed 7 of 7 files at r8.
Reviewable status:
pkg/sql/opt/optbuilder/testdata/select, line 1209 at r7 (raw file):
Previously, madhavsuresh wrote…
There's a panic as well with a table with no columns in optbuilder:
exec-ddl CREATE TABLE no_cols_table () ---- build SELECT * FROM no_cols_table ----The panic is here:
For now I've removed this test from optbuilder, since it causes the tests to panic out.
Thanks for that suggestion - added the test.
That's definitely a bug... not sure when that got introduced, but can you file an issue to address in another PR? Feel free to assign it to me.
madhavsuresh
force-pushed
the
numeric_references
branch
5 times, most recently
from
f5d0f0d to
0bd998e
Compare
There was a problem hiding this comment.
Thanks for the reviews!!
bors r+
Reviewable status:
pkg/sql/opt/optbuilder/testdata/select, line 1209 at r7 (raw file):
Previously, rytaft wrote…
That's definitely a bug... not sure when that got introduced, but can you file an issue to address in another PR? Feel free to assign it to me.
Done.
This commit adds support in the optimizer for the following type of query: `SELECT * FROM [53 as t]` Release note: - Numeric references in opt/ do not use the table descriptor cache. This diverges from how numeric references were resolved in the heuristic planner. - This PR changes the semantics of SELECT * FROM [53() as t]. Previously, and empty column list to a table reference query was accepted, and an zero column row was returned. With this change, an empty column list is no longer accepted. Empty column lists now returns the error: "An explicit list of column IDs must include at least one column" Relevant comment by @knz on this change: "I am not 100% sure how to solve this but I would be comfortable to 'solve' this by rejecting the notation NN() during planning and say with an error "an explicit list of column IDs must include at least one column". Even in the envisioned case for numeric references to handle view descriptors, this would be adequate (a numeric reference inside a view descriptor will always include at least the PK columns, even if only hidden)"
madhavsuresh
force-pushed
the
numeric_references
branch
from
0bd998e to
182770e
Compare
Canceled |
|
bors r+ |
Build failed |
|
bors failure doesn't seem related to this change... bors r+ |
28159: opt: add support for numeric references r=madhavsuresh a=madhavsuresh This commit adds support in the optimizer for the following type of query: `SELECT * FROM [53 as t]` Release note: - Numeric references in opt/ do not use the table descriptor cache. This diverges from how numeric references were resolved in the heuristic planner. - This PR changes the semantics of SELECT * FROM [53() as t]. Previously, and empty column list to a table reference query was accepted, and an zero column row was returned. With this change, an empty column list is no longer accepted. Empty column lists now returns the error: "An explicit list of column IDs must include at least one column" Relevant comment by @knz on this change: "I am not 100% sure how to solve this but I would be comfortable to 'solve' this by rejecting the notation NN() during planning and say with an error "an explicit list of column IDs must include at least one column". Even in the envisioned case for numeric references to handle view descriptors, this would be adequate (a numeric reference inside a view descriptor will always include at least the PK columns, even if only hidden)" 28339: storage: rename ConsistencyServer to StoreMetaServer r=nvanbenschoten,bdarnell a=benesch The ConsistencyServer will soon learn to assist with merges. Specifically, it will gain a WaitForAppliedIndex RPC (or something similar) to block until the specified replica reaches the requested AppliedIndex. As such, it needs a more general name. Rename it to StoreMetaServer. The alternative of creating a separate server for the new RPC seemed like overkill. Release note: None 28394: storageccl: improve support for non-S3 endpoints r=mjibson a=mjibson Remove the requirement for a region if an endpoint is specified by using a bogus default region string. If an endpoint is specified, use the old style S3 urls (http://endpoint/bucket instead of http://bucket.endpoint/). These two changes were tested with Minio and DO Spaces and both now work as expected (without specifying a region and no weird endpoint URL). Fixes #24224 Fixes #21412 Release note (sql change): Improve support for S3-compatible endpoints in BACKUP, RESTORE, and IMPORT. The AWS_REGION parameter is no longer required. Services like Digital Ocean Spaces and Minio now work correctly. Co-authored-by: madhavsuresh <[email protected]> Co-authored-by: Nikhil Benesch <[email protected]> Co-authored-by: Matt Jibson <[email protected]>
Build succeeded |
This commit adds support in the optimizer for the following
type of query:
SELECT * FROM [53 as t]Release note:
cache. This diverges from how numeric references were resolved in the
heuristic planner.
Previously, and empty column list to a table reference query
was accepted, and an zero column row was returned. With this
change, an empty column list is no longer accepted. Empty column
lists now returns the error: "An explicit list of column IDs must
include at least one column"
Relevant comment by @knz on this change:
"I am not 100% sure how to solve this but I would be comfortable to
'solve' this by rejecting the notation NN() during planning and say with
an error "an explicit list of column IDs must include at least one
column".
Even in the envisioned case for numeric references to handle view
descriptors, this would be adequate (a numeric reference inside a view
descriptor will always include at least the PK columns, even if only
hidden)"